Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure sites are always stopped #514

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Ensure sites are always stopped #514

merged 2 commits into from
Sep 9, 2024

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Sep 5, 2024

Related to https://github.com//issues/9018-gh-Automattic/dotcom-forge.

Proposed Changes

  • Ensure the server process is killed independently of the result of the stop message sent to the process. Ideally, we kill the process when the server stops gracefully, but we'll force this if the set timeout is reached.
  • Change the timeout of the server stop message from 2 minutes to 5 seconds. The 2-minute timeout is the default value we use for all server messages. My rationale for this change is that this value seems reasonable for starting the site or requests, but a server should be stopped sooner.
  • Catch any errors produced during the server stop. We'll log the error internally but there won't be any notification to the user. I thought to report the error to Sentry, but since it won't have any context due to being a timeout, I felt it would end up generating noise instead of being helpful.

Testing Instructions

  • Run the app with the command npm start.
  • Create a site.
  • Open the site's folder.
  • Create a file with the name test.php at root with the following content. It will make the request to run indefinitely.
<?php
while (true) {
    sleep(1);
}
  • Start the site.
  • Navigate to http://localhost:<PORT>/test.php.
  • Observe that the page gets in an indefinite loading state.
  • Stop the site.
  • Observe the site is stopped after 5 seconds.
  • Observe an error message is logged to the console.
Before (timeout has been set to 5 seconds for testing) After
studio-server-stop-before.mp4
studio-stop-site-after.mp4

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@fluiddot fluiddot requested a review from a team September 5, 2024 16:41
@fluiddot fluiddot self-assigned this Sep 5, 2024
@fluiddot fluiddot changed the title Fix/force server stop Ensure sites are always stopped Sep 5, 2024
Copy link
Contributor

@kozer kozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @fluiddot ! It works as expected!

@fluiddot fluiddot merged commit 30825b9 into trunk Sep 9, 2024
13 checks passed
@fluiddot fluiddot deleted the fix/force-server-stop branch September 9, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants